Adjust code according to review #1
Conversation
| <property name="connection.connection_string"> | ||
| Server=localhost:39015;UserID=nhibernate;Password= | ||
| Server=localhost:39015;UserID=nhibernate;Password=; | ||
| Enlist=false; |
There was a problem hiding this comment.
This is the way to fix most transaction scope tests failures in my opinion. So this needs to be adjusted in any previously existing test configuration.
| default: | ||
| Assert.That(substringFunction, Is.TypeOf<AnsiSubstringFunction>()); | ||
| break; | ||
| } |
There was a problem hiding this comment.
Rather than adding one more if, I have made use of the new pattern matching switch.
| @@ -18,7 +18,7 @@ protected override string MappingsAssembly | |||
|
|
|||
| protected override bool AppliesTo(Dialect.Dialect dialect) | |||
| { | |||
| return dialect.SupportsSequences && !(dialect is Dialect.MsSql2012Dialect) && !(dialect is Dialect.AbstractHanaDialect); | |||
| return dialect.SupportsSequences && !(dialect is Dialect.MsSql2012Dialect) && !(dialect is Dialect.HanaDialectBase); | |||
There was a problem hiding this comment.
Still remains the question here:
Is there no way to provide a proper
Dialect.AddIdentifierOutParameterToInsertoverride for supporting this? (I think theMsSql2012Dialectis just lacking it here, and should have it indeed. Better not add another case where the override would be just missing.)If there is no solution, please add a comment for telling why HANA is excluded, like "SAP HANA does not support a syntax allowing to return the inserted id as an output parameter or a return value".
| if (Dialect is AbstractHanaDialect) | ||
| { | ||
| Assert.Ignore("feature not supported: Currently specify table name by 'FOR UPDATE of t1.c1' if there are more than one tables/views/subqueries in the FROM clause"); | ||
| } |
There was a problem hiding this comment.
Removing this does not cause any failure in my tests. Was it failing in some earlier version of the dialect, but you would have fix that then forgot about the test change here?
There was a problem hiding this comment.
This is new with HANA 2 SP3. I wrote the dialect for HANA 2 SP2 where it's failing. But I think it's ok to require a minimum of HANA 2 SP3 for NHibernate.
There was a problem hiding this comment.
Well, considering this does not change anything in the NHibernate assembly, this will be a requirement only for the test assembly. So yes, let the tests have this requirement.
| @@ -8,8 +8,7 @@ public class Fixture : BugTestCase | |||
| { | |||
| protected override bool AppliesTo(Dialect.Dialect dialect) | |||
| { | |||
| return !(dialect is Oracle8iDialect) && | |||
| !(dialect is AbstractHanaDialect); | |||
| return !(dialect is Oracle8iDialect); | |||
There was a problem hiding this comment.
Does not fail on my side too. Am I missing something?
There was a problem hiding this comment.
I probably excluded the test at first before I implemented the boolean operators and forgot to include it again afterwards.
| @@ -9,8 +9,7 @@ public class Fixture : BugTestCase | |||
| { | |||
| protected override bool AppliesTo(Dialect.Dialect dialect) | |||
| { | |||
| return !(dialect is Oracle8iDialect) && | |||
| !(dialect is AbstractHanaDialect); | |||
| return !(dialect is Oracle8iDialect); | |||
There was a problem hiding this comment.
Same as previous comment.
| connectionString, $"[^;\"a-zA-Z]*{autoEnlistmentKeywordPattern}=[^;\"]*;?", string.Empty, | ||
| RegexOptions.IgnoreCase); | ||
| // Avoid redundant semi-colon | ||
| connectionString = Regex.Replace( |
There was a problem hiding this comment.
HanaConnection does not support them, so I had to improve this connection string patching.
| @@ -182,7 +182,6 @@ public void ShouldNotifyAfterDistributedTransactionWithOwnConnection(bool doComm | |||
| { | |||
| using (var tx = new TransactionScope()) | |||
| { | |||
| ownConnection1.EnlistTransaction(System.Transactions.Transaction.Current); | |||
There was a problem hiding this comment.
This was undue. The enlistment is done by NHibernate even on provided connections. (When I have worked on the system transaction mess (see NH-4011), I have initially considered not handling transaction enlistment on provided connections, but changed my mind later and forgot about this line here.)
Due to HanaConnection not supporting being asked to enlist in a transaction in which it is already enlisted, the test was failing for HANA.
fredericDelaporte
changed the title
Partially address the review.
Do some more reformatting, fix some more tab/spaces mixup.
|
If merging this, many things to address will remain:
Some of them should be easy to "fix", their error being quite self-explanatory: "correlated subquery cannot have TOP or ORDER BY". Each case I have seen had a top, so adding a |
|
Other cases of failing scalar sub-selects seems to always be "scalar sub select in order by", so a |
Partially address the review of nhibernate#1662.
This increases the number of failing tests. I consider these failing tests need more investigation and another handling than considering some feature not supported while they look indeed to be supported (see review details).